fix(AlertGroup): update AlertGroupInline to not use child index as a key#7878
Conversation
|
Preview: https://patternfly-react-pr-7878.surge.sh A11y report: https://patternfly-react-pr-7878-a11y.surge.sh |
| {React.Children.toArray(children).map((alert, index) => ( | ||
| <li key={(alert as React.ReactElement<AlertProps>).props?.uniqueId || index}>{alert}</li> |
There was a problem hiding this comment.
I've been thinking if it is the best solution to add the uniqueId prop as it is intended only for 'internal' use. Also, it may be a bit confusing for the user when the expected behavior of the alert depends on prop that is optional.
What if we generated e.g. uuid and used it as key instead? This would however require either to use a 3rd party package or write a generator, I've seen a couple of good implementations for example here.
I'm aware that there are some drawbacks when using uuids. Therefore I'll be fine also with uniqueId prop, but wanted to bring up the other option too :)
UPDATE:
We discussed this suggestion off GitHub and it looks like uuid doesn't solve the problem.
I agree with adding the uniqueId prop.
| /** Adds accessible text to the alert Toggle */ | ||
| toggleAriaLabel?: string; | ||
| /** Uniquely identifies the alert */ | ||
| uniqueId?: string; |
…key (patternfly#7878) * fix(AlertGroup): update AlertGroupInline to not use child index as a key * add index as backup key in case an alert is missing a uniqueId * add tests * change prop name uniqueId -> id
What: Closes #7877
Additional issues:
Needed for #7843
Note: I added an integration test rather than unit tests as I couldn't identify a good way to unit test either side of the Alert/AlertGroup/AlertGroupInline interaction. This is because Alert doesn't do anything with
uniqueIdandAlertGroupInlinejust applies it as akey.